Repository navigation
fix(desktop): retain new-workspace image attachments across navigation - #6282
Merged
Merged
Conversation
The /new-workspace route mounted PromptInputProvider inside the route, so navigating away dropped the attachment state and revoked every blob URL, while the prompt text survived in the module-level draft store. PromptInputProvider now accepts an opt-in attachmentsStore that moves attachment state into a module-scoped external store (blob URLs are then owned by the store, not revoked on unmount). All three new-workspace create surfaces share one singleton store, cleared on draft reset. The filename-to-source-path map for Finder reveal is hoisted with it.
Contributor
📝 WalkthroughWalkthroughThe PR adds an external attachment store to ChangesNew-workspace attachment persistence
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant NewWorkspaceScreen
participant PromptInputProvider
participant newWorkspaceAttachmentsStore
participant DraftState
NewWorkspaceScreen->>PromptInputProvider: add attachment
PromptInputProvider->>newWorkspaceAttachmentsStore: store attachment item
NewWorkspaceScreen->>newWorkspaceAttachmentsStore: store source path
DraftState->>newWorkspaceAttachmentsStore: change resetKey
newWorkspaceAttachmentsStore->>newWorkspaceAttachmentsStore: clear attachments and source paths
newWorkspaceAttachmentsStore-->>PromptInputProvider: notify subscribers
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/stores/new-workspace-attachments.test.ts (1)
16-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests cover the store contract but not the provider-unmount claim.
The four tests verify persistence, reset cleanup, URL revocation, and subscriber notification. The central behavior of the PR is that
PromptInputProviderskips blob URL revocation on unmount whenattachmentsStoreis set. No test asserts that.That test belongs next to
PromptInputProviderinpackages/ui, because it needs to mount and unmount the provider. Add a case that mounts the provider with a store, adds a file, unmounts, and assertsURL.revokeObjectURLwas not called.Do you want me to draft that test?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/stores/new-workspace-attachments.test.ts` around lines 16 - 63, Add a provider-level test alongside PromptInputProvider in packages/ui that mounts the provider with an attachmentsStore, adds an attachment, unmounts it, and asserts URL.revokeObjectURL was not called. Keep the test isolated by restoring the revokeObjectURL mock and cleaning up the mounted provider afterward.apps/desktop/src/renderer/stores/new-workspace-attachments.ts (1)
12-17: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove path entries when attachments are removed.
newWorkspaceAttachmentPathsis keyed by filename and only cleared onresetDraft. Removing a card callsattachments.remove(id), leaving the path keyed byfile.filename; a later same-named file can be poisoned if it comes from a different path. Delete/clear the corresponding entry onattachments.remove(), or key the map by attachmentid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/stores/new-workspace-attachments.ts` around lines 12 - 17, Update the attachment removal flow around attachments.remove() to delete the removed attachment’s corresponding entry from newWorkspaceAttachmentPaths using its filename, while preserving resetDraft’s full-map clearing behavior. Ensure removal occurs before or alongside the card deletion so a later same-named file cannot reuse stale path data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/desktop/src/renderer/stores/new-workspace-attachments.test.ts`:
- Around line 16-63: Add a provider-level test alongside PromptInputProvider in
packages/ui that mounts the provider with an attachmentsStore, adds an
attachment, unmounts it, and asserts URL.revokeObjectURL was not called. Keep
the test isolated by restoring the revokeObjectURL mock and cleaning up the
mounted provider afterward.
In `@apps/desktop/src/renderer/stores/new-workspace-attachments.ts`:
- Around line 12-17: Update the attachment removal flow around
attachments.remove() to delete the removed attachment’s corresponding entry from
newWorkspaceAttachmentPaths using its filename, while preserving resetDraft’s
full-map clearing behavior. Ensure removal occurs before or alongside the card
deletion so a later same-named file cannot reuse stale path data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 395e5c0d-f395-43fa-aa8d-f53b6d5c3416
📒 Files selected for processing (7)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/new-workspace/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/components/NewWorkspaceEmptyScreen/NewWorkspaceEmptyScreen.tsxapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/DashboardNewWorkspaceModal.tsxapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/NewWorkspaceScreen/NewWorkspaceScreen.tsxapps/desktop/src/renderer/stores/new-workspace-attachments.test.tsapps/desktop/src/renderer/stores/new-workspace-attachments.tspackages/ui/src/components/ai-elements/prompt-input.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/ui/src/components/ai-elements/prompt-input.tsx">
<violation number="1" location="packages/ui/src/components/ai-elements/prompt-input.tsx:170">
P3: `get: () => files` returns the store's internal mutable array directly. Because useSyncExternalStore relies on get() returning a value that only changes when the store is actually updated, any consumer that mutates the returned array in place would bypass notify() — subscribers wouldn't re-render and React would see a stale snapshot. Today all internal call sites reassign the array via set, so it isn't triggered yet, but returning a defensive copy makes the store robust and keeps the external-API contract explicit that updates must go through set/clear.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Surf as Create Surfaces (page/empty/modal)
participant Prov as PromptInputProvider
participant AttStore as NewWorkspaceAttachmentsStore
participant Paths as NewWorkspaceAttachmentPaths
participant DraftStore as useNewWorkspaceDraftStore
Note over Surf,DraftStore: NEW: Attachments persist across navigation via module-scoped store
Surf->>Prov: mount with attachmentsStore
Prov->>AttStore: subscribe + get() (useSyncExternalStore)
AttStore-->>Prov: current files (or [])
alt User adds/drops files
Surf->>Prov: add(files)
Prov->>AttStore: set(prev.concat(items)) + create object URLs
Surf->>Paths: set(filename, sourcePath) for Finder reveal
end
Note over Surf,Prov: Navigate away/back (provider unmounts/remounts)
Prov->>Prov: Do NOT revoke URLs (store owns lifetime)
Prov->>AttStore: re-subscribe/get on remount
AttStore-->>Prov: files still present
Note over Surf,DraftStore: On successful create or explicit discard
Surf->>DraftStore: resetDraft()
DraftStore->>DraftStore: resetKey changes
DraftStore-->>AttStore: module-level subscription notified
AttStore->>AttStore: clear() + revoke all blob URLs
Paths->>Paths: clear()
AttStore-->>DraftStore: reset complete
Note over AttStore,Surf: Stable attachment IDs keep upload cache entries valid (no re-upload)
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
| }; | ||
| return { | ||
| get: () => files, |
Contributor
There was a problem hiding this comment.
P3: get: () => files returns the store's internal mutable array directly. Because useSyncExternalStore relies on get() returning a value that only changes when the store is actually updated, any consumer that mutates the returned array in place would bypass notify() — subscribers wouldn't re-render and React would see a stale snapshot. Today all internal call sites reassign the array via set, so it isn't triggered yet, but returning a defensive copy makes the store robust and keeps the external-API contract explicit that updates must go through set/clear.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/ui/src/components/ai-elements/prompt-input.tsx, line 170:
<comment>`get: () => files` returns the store's internal mutable array directly. Because useSyncExternalStore relies on get() returning a value that only changes when the store is actually updated, any consumer that mutates the returned array in place would bypass notify() — subscribers wouldn't re-render and React would see a stale snapshot. Today all internal call sites reassign the array via set, so it isn't triggered yet, but returning a defensive copy makes the store robust and keeps the external-API contract explicit that updates must go through set/clear.</comment>
<file context>
@@ -141,8 +142,61 @@ export const useProviderAttachments = () => {
+ }
+ };
+ return {
+ get: () => files,
+ set: (next) => {
+ files = next;
</file context>
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On the v2 new-workspace page (
/new-workspaceroute and the empty-dashboard create screen), typed prompt text survives navigating away and back, but attached images vanish.The text lives in the module-level
useNewWorkspaceDraftStore; attachments lived in React state insidePromptInputProvider, which those surfaces mount inside the route. Navigating away unmounts the provider, dropping the file list and revoking every blob URL. The control-arm modal never had the bug only because its provider sits in the always-mounted layout.Fix
packages/uiprompt-input.tsx:PromptInputProvideraccepts an opt-inattachmentsStore(created viacreatePromptInputAttachmentsStore()). When provided, attachment state lives in that module-scoped store and blob URLs are no longer revoked on provider unmount — the store owns their lifetime. Without the prop, behavior is unchanged.new-workspace-attachments.ts(new): singleton store wired into all three create surfaces (route, empty screen, modal — both experiment arms now share one draft, matching the text). A subscription on the draft store'sresetKeyclears attachments + revokes URLs on successful create/discard, even while no create surface is mounted.NewWorkspaceScreen: the filename→source-path map backing "Reveal in Finder" was also component-local; hoisted into the same module and cleared on reset.Attachment ids survive navigation, so the idempotent upload cache (keyed
(fileId, hostUrl)) keeps its entries — no re-upload on return, and the per-host visibility filter still matches.Testing
new-workspace-attachments.test.ts(4 cases): persistence, reset clearing + URL revocation, subscriber notification. Verified the reset test fails when the reset handler is mutated.bun run lint, andpackages/uitests pass.Summary by cubic
Fixes lost image attachments on the v2 new-workspace screens. Attachments now persist across navigation and only clear on successful create or discard.
Bug Fixes
resetKeysubscription; also hoist and clear the filename→source-path map for “Reveal in Finder.”New Features
PromptInputProviderin@superset/ui/ai-elements/prompt-inputaccepts an optionalattachmentsStore.createPromptInputAttachmentsStore()to hold attachment state outside the provider; blob URLs are not revoked on provider unmount when used. Behavior is unchanged when not provided.Written for commit e7a974f. Summary will update on new commits.